Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new lint upper_case_acronyms #6475

Merged
merged 3 commits into from
Jan 20, 2021

Conversation

hkmatsumoto
Copy link
Member

@hkmatsumoto hkmatsumoto commented Dec 19, 2020

Close #1335
I need some reviews on the English sentences because I feel they're messed up. ;)

changelog: Add new lint upper_case_acronyms

@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 19, 2020
@ghost
Copy link

ghost commented Dec 20, 2020

CCHelperCcHelper and SAOnlySaOnly looks pretty weird to me. In .Net, at least, two letter acronyms are in uppercase. See .Net documenation - Capitalization Conventions. The issue talks about VS plugins so those are probably the rules they are using. I tried to find some Rust documentation on capitalization conventions of acronyms but I didn't find anything.

Also, I think this might have to be pedantic, as it will generate excessive warnings in some projects.

@hkmatsumoto
Copy link
Member Author

CCHelper → CcHelper and SAOnly → SaOnly looks pretty weird to me. In .Net, at least, two letter acronyms are in uppercase. See .Net documenation - Capitalization Conventions. The issue talks about VS plugins so those are probably the rules they are using. I tried to find some Rust documentation on capitalization conventions of acronyms but I didn't find anything.

I have the same spontaneous feeling on two-letter acronyms but IMO they aren't exceptions in Rust given IoSlice, TypeId and GlObject.

Also, I think this might have to be pedantic, as it will generate excessive warnings in some projects.

If two-letter acronyms are exceptions pedantic is fair as the lint will be false-positive (including ID and IO) prone. If not I'd prefer this lint to be deny-by-default since the lint should be as broad as possible, considering the naming convention on acronyms are not intuitive for those who came from other languages.

@ghost
Copy link

ghost commented Dec 21, 2020

The IoSlice example convinced me that two letter acronyms aren't an exception.

I'll let the reviewer decide on whether this should be pedantic or something else. I tested it on a bunch of crates and there weren't that many warnings.

The one issue I found is that it lints on globals created with lazy_static.

use lazy_static::lazy_static;

lazy_static! {
    /// This is an example for using doc comment attributes
    static ref EXAMPLE: u8 = 42;
}

fn main() {}
error: name `EXAMPLE` may contain capitalized acronym
  --> src/main.rs:11:16
   |
11 |     static ref EXAMPLE: u8 = 42;
   |                ^^^^^^^ help: consider making the acronym lowercase expect the initial letter: `Example`
   |
note: the lint level is defined here
  --> src/main.rs:1:9
   |

There probably needs to be a macro check somewhere.

I was also wondering if we should call the lint upper-case-acronyms to be consistent with the non-upper-case-globals rust lint.

@hkmatsumoto
Copy link
Member Author

hkmatsumoto commented Dec 21, 2020

I'll let the reviewer decide on whether this should be pedantic or something else. I tested it on a bunch of crates and there weren't that many warnings.

Got it. I guess I need opinions from others on its lint category.

The one issue I found is that it lints on globals created with lazy_static.

Very nice catch! lazy_static expands EXAMPLE to a struct, leading to being linted. Perhaps this lint should ignore macros.
I feel upper-case-acronyms is nicer than capitalized-acronyms, I'll rename it to that.

I'll make the changes next week as I have a lot to do right now.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think style is appropriate for this lint.

I tried to find some Rust documentation on capitalization conventions of acronyms but I didn't find anything.

@mikerite The API guidelines of rust mention this in the paragraph after the table, you can find here: https://rust-lang.github.io/api-guidelines/naming.html#casing-conforms-to-rfc-430-c-case

@matsujika ^ Please link this in the link documentation instead, since it is more current.

clippy_lints/src/capitalized_acronyms.rs Outdated Show resolved Hide resolved
clippy_lints/src/capitalized_acronyms.rs Outdated Show resolved Hide resolved
clippy_lints/src/capitalized_acronyms.rs Outdated Show resolved Hide resolved
clippy_lints/src/capitalized_acronyms.rs Outdated Show resolved Hide resolved
tests/ui/complex_types.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 27, 2020
@hkmatsumoto

This comment has been minimized.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 29, 2020
@hkmatsumoto hkmatsumoto changed the title Add new lint capitalized_acronyms Add new lint upper_case_acronyms Jan 1, 2021
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small things left

clippy_lints/src/upper_case_acronyms.rs Outdated Show resolved Hide resolved
/// ```rust
/// struct HttpResponse;
/// ```
pub UPPER_CASE_ACRONYMS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub UPPER_CASE_ACRONYMS,
pub UPPERCASE_ACRONYMS,

Uppercase is usually written as one word. Like in the char::to_uppercase method

Copy link
Member Author

@hkmatsumoto hkmatsumoto Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's non-upper-case-globals lint in rustc and I suppose it's reasonable to make the name of this lint similar to that. (It seems they wanted to go consistent with non-camel-case-types and non_snake_case when naming the rustc lint)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I find that inconsistent. But lets be consistently inconsistent and keep the name as it is 👍

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 4, 2021
@hkmatsumoto

This comment has been minimized.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 9, 2021
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Please squash some of your commits, so we can merge this.

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 17, 2021
clippy_lints/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Philipp Krones <hello@philkrones.com>
@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Jan 20, 2021

📌 Commit 0ccb491 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Jan 20, 2021

⌛ Testing commit 0ccb491 with merge e6665e4...

@bors
Copy link
Collaborator

bors commented Jan 20, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing e6665e4 to master...

@bors bors merged commit e6665e4 into rust-lang:master Jan 20, 2021
@hkmatsumoto hkmatsumoto deleted the capitalized_acronyms branch January 20, 2021 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Naming convention
5 participants